Clarify semantics when port_redirect is unset #1740
Conversation
Relates to kubernetes-sigs#1725 Signed-off-by: Arko Dasgupta <[email protected]>
k8s-ci-robot
added
kind/documentation
|
Hi @arkodg. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
needs-ok-to-test
Signed-off-by: Arko Dasgupta <[email protected]>
|
Thanks @arkodg! We definitely need to clarify the spec here. If we're not careful, we may end up breaking some implementations along the way, so need to make sure we get as much feedback as possible before merging. Adding a hold until we've had more implementers weigh in. /cc @skriss @shaneutt @youngnick |
k8s-ci-robot
added
the
ok-to-test
k8s-ci-robot
added
do-not-merge/hold
Signed-off-by: Arko Dasgupta <[email protected]>
|
@shaneutt updated based on the guidance received by @youngnick yesterday |
|
cross linking this slack thread where a user was unable to redirect correctly because the default port 443 was getting appended to the |
Signed-off-by: Arko Dasgupta <[email protected]>
shaneutt
dismissed
their stale review
removing my block, I didn't intend to block.
Signed-off-by: Arko Dasgupta <[email protected]>
|
/approved /hold for other review though |
k8s-ci-robot
removed
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arkodg, shaneutt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
| // When empty, the Gateway Listener port is used. | ||
| // In all cases, when the protocol is http and the port is port 80, | ||
| // then the port must be omitted. Similarly, when the protocol is | ||
| // https and the port is port 443, then the port must also be omitted. |
There was a problem hiding this comment.
should I change must to will so it caters to both parties ?
There was a problem hiding this comment.
I think what we're trying to say is the following:
If the port is not specified in the Redirect Filter, implementations should use the port from
the listener for redirects _unless_ the Listener or Redirect Filter protocol is HTTP or HTTPS,
in which case it should not specify a port in the redirect configuration.
I'm not actually sure that's what we're trying to say, and what I wrote above is still rather confusing, but maybe it's at least clear that the guidance is intended for implementers?
There was a problem hiding this comment.
suggest reading through this thread #1740 (comment) and sharing your feedback, we are saying ports 80 and 443 should never be appended to the Location header
There was a problem hiding this comment.
I think that overall idea makes sense, it's just not clear to me in the current variation of the spec, moving this to #1806 so we don't lose track of the discussion.
Relates to #1725
/kind documentation